Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit & Dyn_emit now wrap string.format + Vector instruction segment fix #27

Merged

Conversation

DerelictDrone
Copy link
Member

@DerelictDrone DerelictDrone commented Nov 26, 2023

May fix #26 for vector instructions, tested with VADD but needs further testing on the other VEXP instructions.

Implements @Vurv78's suggestion for ZVM:Emit() and ZVM:Dyn_Emit(), since they take strings, making them wrap string.format() to stop concatenating strings and numbers in a potentially unsafe manner.

Converts all known calls to dyn_emit and emit that concatenate numbers in zvm_opcodes and gmod_wire_cpu to use %d

Dyn_EmitOperand remains unaffected at the moment due to taking multiple args already

Comment on lines 1235 to 1240
if not self.EmitOperandSegment[1] then
seg1code = "VM.DS"
end
if not self.EmitOperandSegment[2] then
seg2code = "VM.DS"
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't seg1code and seg2code be self.EmitOperandSegment[1] and self.EmitOperandSegment[2] if they are set?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EmitOperandSegment needs to be checked for validity if that's the case otherwise it still poses a threat.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for fixing #26, which is that if they have a segment set it would add the segment to it twice, because if say, FS = 6 and R0 = 2, passing FS:R0 to these instructions is the same as passing 8, but these instructions expect it to just be the value of R0 rather than the value of FS + R0, so it would end up trying to load a vector from memory index 14(6+8) rather than index 8(6+2)

The only time this isn't the case is when a segment isn't used, where it instead defaults to adding VM.DS, because the value of the operand isn't getting segment added to it by default, so I default it to adding 0 if there's a segment in that spot to avoid adding it twice without breaking the functionality of non-memory uses of segment:reg offsetting, because reading variables in functions use this trick(they do RSTACK REG,EBP:const a lot for 3 byte access that doesn't modify the ptr register)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay, makes sense

@thegrb93
Copy link
Contributor

thegrb93 commented Nov 27, 2023

Looks like an improvement, but is a huge cost to performance as well. I'd like to see the 'Emit' shit replaced with actual lua rather than string-building into compiled lua code at some point. This is fine for now at least.

Comment on lines 1234 to 1240
local seg1code,seg2code = "0","0"
if not self.EmitOperandSegment[1] then
seg1code = "VM.DS"
end
if not self.EmitOperandSegment[2] then
seg2code = "VM.DS"
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these all be changed to this?

local seg1code = self.EmitOperandSegment[1] and "0" or "VM.DS"
local seg2code = self.EmitOperandSegment[2] and "0" or "VM.DS"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like that works, so sure, I'll get a commit for it in a moment.

@DerelictDrone
Copy link
Member Author

Looks like an improvement, but is a huge cost to performance as well. I'd like to see the 'Emit' shit replaced with actual lua rather than string-building into compiled lua code at some point. This is fine for now at least.

I have been wondering if there may be a performance improvement in reducing the number of calls to string format and global emit/dyn_emit, by possibly replacing them with a version that builds a smaller, local string(saving the arguments), then calling the global emit at the end which will run string.format once on the whole instruction's code rather than string.formatting and concatenating to the entire block on every emit/dyn_emit

I'd like to ask others that are more familiar with lua's performance regarding the local part, but I imagine less frequent (but bigger) calls to string.format would go a long way

@thegrb93
Copy link
Contributor

Yeah the less concats and formats, the better.

@thegrb93
Copy link
Contributor

You could potentially combine a bunch of those groups of emits into a single emit.

@Vurv78
Copy link
Contributor

Vurv78 commented Nov 27, 2023

Looks like an improvement, but is a huge cost to performance as well. I'd like to see the 'Emit' shit replaced with actual lua rather than string-building into compiled lua code at some point. This is fine for now at least.

Didn't realize it ran at runtime. That is pretty terrible. Then again the emit function itself is very laggy just looking at the code. It uses repeated string concatenation so a string.format call is the least of our problems considering the huge amount of allocations this alone is causing.

If it were me I'd refactor it to store it properly as a string buffer and only table.concat when it's needed, and replace the varargs with parameters a-f so the code can jit compile.

Copy link
Contributor

@Vurv78 Vurv78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should change this emit function too, no?

function ZVM:Emit(text)

@DerelictDrone
Copy link
Member Author

DerelictDrone commented Nov 28, 2023

Should change this emit function too, no?

function ZVM:Emit(text)

@Vurv78 That one can only occur if microcodedebug is on, the only condition that sets it is if not SERVER and not CLIENT, I don't know if such a state is possible unless you're running it outside of gmod, or manually assigning it

@DerelictDrone
Copy link
Member Author

DerelictDrone commented Nov 28, 2023

Looks like an improvement, but is a huge cost to performance as well. I'd like to see the 'Emit' shit replaced with actual lua rather than string-building into compiled lua code at some point. This is fine for now at least.

Didn't realize it ran at runtime. That is pretty terrible. Then again the emit function itself is very laggy just looking at the code. It uses repeated string concatenation so a string.format call is the least of our problems considering the huge amount of allocations this alone is causing.

I'm fairly certain these will only run on compile/precompile, I.E that section is only encountered for the first time and hasn't been modified by the CPU itself

I've noticed that you can tell easily when something like the GPU is starting a precompile by it flashing the "took too long to draw frame!" the first time you enter a new block.

If it were me I'd refactor it to store it properly as a string buffer and only table.concat when it's needed, and replace the varargs with parameters a-f so the code can jit compile.

If you think this'll help I can do it.

@Vurv78
Copy link
Contributor

Vurv78 commented Nov 28, 2023

Should change this emit function too, no?

function ZVM:Emit(text)

@Vurv78 That one can only occur if microcodedebug is on, the only condition that sets it is if not SERVER and not CLIENT, I don't know if such a state is possible unless you're running it outside of gmod, or manually assigning it

Then if it's unused you could remove it or add a comment saying so and leave it for later

@Vurv78
Copy link
Contributor

Vurv78 commented Nov 28, 2023

Looks like an improvement, but is a huge cost to performance as well. I'd like to see the 'Emit' shit replaced with actual lua rather than string-building into compiled lua code at some point. This is fine for now at least.

Didn't realize it ran at runtime. That is pretty terrible. Then again the emit function itself is very laggy just looking at the code. It uses repeated string concatenation so a string.format call is the least of our problems considering the huge amount of allocations this alone is causing.

I'm fairly certain these will only run on compile/precompile, I.E that section is only encountered for the first time and hasn't been modified by the CPU itself

I've noticed that you can tell easily when something like the GPU is starting a precompile by it flashing the "took too long to draw frame!" the first time you enter a new block.

If it were me I'd refactor it to store it properly as a string buffer and only table.concat when it's needed, and replace the varargs with parameters a-f so the code can jit compile.

If you think this'll help I can do it.

If it's not runtime then it doesn't really matter, so up to you. I just skimmed over the code because @thegrb93 said it'd be a perf hit. I did initially think it only ran to compile once

@DerelictDrone
Copy link
Member Author

I should probably test to check if the emits are run every time, or if they only build a precompiled block, I'm fairly certain it's only while building a precompiled block but it would also be good to see how frequently blocks get precompiled to see how big a hit to performance this may result in.

@DerelictDrone DerelictDrone requested a review from Vurv78 November 28, 2023 00:59
@thegrb93
Copy link
Contributor

They are precompiled blocks, but it explains the tons of lag you get when initially placing a cpu

@DerelictDrone
Copy link
Member Author

DerelictDrone commented Nov 28, 2023

I've got a basic test to measure the amount of time spent in the precompile functions on two separate branches, one before these changes, and one after these changes

Before:
srcds_0OeWryIVQs

After:
image

Confirm for yourselves using these two branches
perf-testing (before)
perf-test-post-emit (after)

@thegrb93
Copy link
Contributor

I wasn't concerned with a huge latency increase, just knew it would be slightly more expensive. I imagine most of the latency is from reallocations.

@thegrb93
Copy link
Contributor

I think vurv just wants a comment added to that emit function and this can be merged.

@DerelictDrone
Copy link
Member Author

Marked them all w/ a comment

@thegrb93 thegrb93 merged commit 52f9dcc into wiremod:master Nov 30, 2023
1 check failed
@DerelictDrone DerelictDrone deleted the fix-segments-for-vectors+convert-emits branch December 1, 2023 00:15
DerelictDrone pushed a commit to DerelictDrone/wire-cpu that referenced this pull request Dec 10, 2023
This was referenced Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instructions that handle segment:pointer adds segment to address twice
3 participants